-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Compute): Use paginated lists. #1445
feat(Compute): Use paginated lists. #1445
Conversation
Zone = zone, | ||
}; | ||
do | ||
await foreach(var instance in client.ListAsync(projectId, zone)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to await foreach here would be to await client.ListAsync(projectId, zone).ToListAsync()
to come up with allInstances
, then synchronously iterate over that, but I'm not bothered either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one gives you he chance to break in the middle of the async iteration without having fetched everything, right? Even if we are not showing that,...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. (And will show results as it goes, of course.) I just twitch at manually adding to the list ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that's for the purposes of the tests only, but I do see what you mean about that making you twitch. We have several tests that do this kind of thing across the repo. It's the alternative to testing against stdout.
Here is the summary of changes. You are about to add 2 region tags.
This comment is generated by snippet-bot.
|
@rsamborski I've added the two new samples, PTAL. Thanks. |
// Make the request to list all non-deprecated images in a project. | ||
ListImagesRequest request = new ListImagesRequest | ||
{ | ||
Project = projectId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxResults = 2,
Kindly add MaxResults parameter, which tells the number of images to be displayed in each page.
If not added, all images will be displayed in a single page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not added, all images will be displayed in a single page.
Not really, MaxResults has a default value of 500 as specified on API ref docs
That said, I'm completely fine with adding an explicit MaxResults
for demonstration purposes but I don't think it should have a value of 2
, this is a very low page size value, and will provoke plenty of requests to the server for anyone listing even a few images.
I'm not even worried about our sample tests, I'm worried about users copying this code as is and including it in their projects, and then generating all this unnecessary trafic for their own application.
I'll set to 100
, and I'm happy to adjust it some more. Also, please, consider adjusting this value to something far greater than 2
for samples in other languages as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used 10 in PHP and Python for the paginated results as this seemed to be a good number for demonstration purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 10 seems better, I did list sevaral of the public image sets and couldn't find any set containing more than 200 non-deprecated images. I'll change here to 10, but leave the fully automatic one on 100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Amanda for the explanation! I understand the rational behind the param setting. Will double check the param values in other languages as well.
// Make the request to list all non-deprecated images in a project. | ||
ListImagesRequest request = new ListImagesRequest | ||
{ | ||
Project = projectId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxResults = 2,
Kindly add MaxResults parameter, which tells the number of images to be displayed in each page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response as below, I've set it to MaxResult = 100
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with 100 here, because we are not showing results page by page and just using this under the hood to generate multiple requests.
// Listing only non-deprecated images to reduce the size of the reply. | ||
Filter = "deprecated.state != DEPRECATED" | ||
}; | ||
// Calling AsRawResponses will return a sequence of pages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Calling AsRawResponses will return a sequence of pages. | |
// Use the `AsRawResponses` attribute of returned iterable to have more granular control of | |
// iteration over paginated results from the API. Each time you want to access the | |
// next page, the library retrieves that page from the API. |
Suggesting the comment for standardisation across languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but changed slightly for idiomacity:
- AsRawResponses is a method not a property (or attribute in Java), so it's more accurate to say
Call the AsRawResponse() method...
- iterable => sequence
- distinguishing between the first sequence and the second. The first is the images sequence and the second is the page sequence.
I've change it to:
// Call the AsRawResponses() method of the returned image sequence to access the page sequece instead
// This allows you to have more granular control of iteration over paginated results from the API.
// Each time you request the next element on the page sequence, the library retrieves that page from the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for changing.
// Listing only non-deprecated images to reduce the size of the reply. | ||
Filter = "deprecated.state != DEPRECATED" | ||
}; | ||
await foreach (var image in client.ListAsync(request)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Although the `MaxResults` parameter is specified in the request, the iterable returned
// by the `ListAsync()` method hides the pagination mechanic. The library makes multiple
// requests to the API for you, so you can simply iterate over all the images.
Kindly add the above comment. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, with a slight word change, becuse it's more idiomatic: iterable => sequence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Amanda!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Amanda. LGTM once Sita's comments are addressed.
compute/api/Compute.Samples.Tests/ListImagesPerPageAsyncTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review feedback addressed, PTAL @Sita04 , thanks!
compute/api/Compute.Samples.Tests/ListImagesPerPageAsyncTest.cs
Outdated
Show resolved
Hide resolved
// Make the request to list all non-deprecated images in a project. | ||
ListImagesRequest request = new ListImagesRequest | ||
{ | ||
Project = projectId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not added, all images will be displayed in a single page.
Not really, MaxResults has a default value of 500 as specified on API ref docs
That said, I'm completely fine with adding an explicit MaxResults
for demonstration purposes but I don't think it should have a value of 2
, this is a very low page size value, and will provoke plenty of requests to the server for anyone listing even a few images.
I'm not even worried about our sample tests, I'm worried about users copying this code as is and including it in their projects, and then generating all this unnecessary trafic for their own application.
I'll set to 100
, and I'm happy to adjust it some more. Also, please, consider adjusting this value to something far greater than 2
for samples in other languages as well.
// Make the request to list all non-deprecated images in a project. | ||
ListImagesRequest request = new ListImagesRequest | ||
{ | ||
Project = projectId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response as below, I've set it to MaxResult = 100
.
// Listing only non-deprecated images to reduce the size of the reply. | ||
Filter = "deprecated.state != DEPRECATED" | ||
}; | ||
await foreach (var image in client.ListAsync(request)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, with a slight word change, becuse it's more idiomatic: iterable => sequence
// Listing only non-deprecated images to reduce the size of the reply. | ||
Filter = "deprecated.state != DEPRECATED" | ||
}; | ||
// Calling AsRawResponses will return a sequence of pages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but changed slightly for idiomacity:
- AsRawResponses is a method not a property (or attribute in Java), so it's more accurate to say
Call the AsRawResponse() method...
- iterable => sequence
- distinguishing between the first sequence and the second. The first is the images sequence and the second is the page sequence.
I've change it to:
// Call the AsRawResponses() method of the returned image sequence to access the page sequece instead
// This allows you to have more granular control of iteration over paginated results from the API.
// Each time you request the next element on the page sequence, the library retrieves that page from the API.
Updates Google.Cloud.Compute.V1 dependency. Closes GoogleCloudPlatform#1438
Thanks for making the changes, @amanda-tarafa! LGTM |
Thanks! Failures in CI are unrelated, will force merge. |
Updates Google.Cloud.Compute.V1 dependency.
Closes #1438